Skip to content

Update build_summary_table function #578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rizaon
Copy link
Contributor

@rizaon rizaon commented Mar 11, 2025

This patch update build_summary_table to match the same function in impala-shell.
https://github.com/apache/impala/blob/a07bf84/shell/impala_client.py#L113

Testing:
Run and pass following command

tox -- -ktest_build_summary_table

This patch update build_summary_table to match the same function in
impala-shell.
https://github.com/apache/impala/blob/a07bf84/shell/impala_client.py#L113

Testing:
Run and pass following command
```
tox -- -ktest_build_summary_table
```
setattr(agg_stats, attr, getattr(agg_stats, attr) + val)
setattr(max_stats, attr, max(getattr(max_stats, attr), val))

if node.exec_stats is not None and node.exec_stats:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't be simply if node.exec_stats: enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced this with a new exec_summary.py from apache/impala@e73e2d4

memory_used = max_stats.memory_used
memory_est = est_stats.memory_used
if (is_prettyprint):
avg_time = prettyprint_time(avg_time)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably it would be better to do it in another commit, but I think that formatting could be move to another file logic, e.g. exec_summary.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Copy from apache/impala@e73e2d4

'{0}.{1} a'.format(tmp_db_lower, empty_table)],
]
validate_summary_table(output_dop_0, expected_dop_0)
cur.close_operation()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cur.close_operation() is not needed, execute() (and the cur fixture after the test is finished) will call it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rizaon rizaon requested a review from csringhofer March 28, 2025 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants